-
Notifications
You must be signed in to change notification settings - Fork 193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add ExecutableSource credentials #525
Conversation
46fca69
to
024c8ce
Compare
) { | ||
$json = json_decode($outputFileContents, true); | ||
if (isset($json['expiration_time']) && time() < $json['expiration_time']) { | ||
return $this->parseTokenFromResponse($outputFileContents); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: If there is an error here, the messages won't specify that it comes from reading an output file vs running the executable directly right? Might be helpful to differentiate that for the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if the file response is valid Json but unsuccessful what happens here? In that case we want to re-run the executable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is an error here, the messages won't specify that it comes from reading an output file vs running the executable directly right?
That's correct. It should be easy to add that
if the file response is valid Json but unsuccessful what happens here?
Good question, I believe it throws an error same as if it's the response. Are you suggesting that if the file JSON contains success: false
, we should silently retry the executable?
Is there a good document that lists the expected logic for the outputFile
, because it's not very intuitive. I followed the NodeJS implementation, although I may have missed some things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go/pluggable-auth-design but it doesn't look like that has a great flow diagram. But yes, the expected behavior is that if the output file is parseable (I.e. good JSON) but success:false, then we would re-run the executable instead of just returning the error again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have manually run the tests in go/pluggable-auth-sdk-testing and they are now passing
Co-authored-by: aeitzman <[email protected]>
…is/google-auth-library-php into add-executable-credentialsource
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks good to me from PHP side, just a small nit regarding the tests : )
Great work @bshaffer!
addresses #523